EIP4844: Update cryptography API and Fiat-Shamir logic #3038

asn-d6 · 2022-10-14T21:12:10Z

This PR does a few things with the cryptography of EIP-4844.

Most importantly, it changes the public API of the KZG library to the following high-level API:

- verify_kzg_proof()
- compute_aggregate_kzg_proof()
- verify_aggregate_kzg_proof()
- blob_to_kzg_commitment()

compared to the old much more low-level API:

- compute_powers()
- matrix_lincomb()
- lincomb()
- bytes_to_bls_field()
- evaluate_polynomial_in_evaluation_form()
- verify_kzg_proof()
- compute_kzg_proof()

This means that all the cryptographic logic (including Fiat-Shamir) is now isolated and hidden in the KZG library and the validator.md file ends up being significantly simplified, only calling high-level KZG functions.

Some additional things that this PR does:

Moves all EIP4844 cryptography into polynomial-commitments.md
Improves the Fiat-Shamir stack by removing the need for SSZ and by introducing domain separators.

Future TODO tasks:

Add unittests in test_validator.py that tests the high-level API

Co-authored-by: Kevaundray Wedderburn <kevtheappdev@gmail.com>

specs/eip4844/polynomial-commitments.md

kevaundray · 2022-10-16T04:41:58Z

Closes #3026

djrtwo

Good separation of concerns 👍

can you give more on the debate over ssz vs domain separators vs neither?

kevaundray · 2022-10-16T21:41:46Z

Good separation of concerns 👍

can you give more on the debate over ssz vs domain separators vs neither?

Currently we all agree to remove ssz from the Fiat-Shamir function as this allows for client teams to not implement any of the cryptography.

The debate is on whether these separator labels that we append just before we add a serialised point or field element or polynomial into the bytes array, add any extra security vs having no label at all.

The argument for having them is that you want a precise mapping from your higher level objects to the bytes array. For example, if I was adding a point into the byte array, I could simply serialise the point as 48 bytes then append onto the byte array, or I could first append a label "POINT" and then add the 48 bytes. In this way, the byte array also encodes the types.

Although this seems like a reasonable thing to do, there is no strong rationale as to whether it improves security, and seems to lean on the side of being "extra safe"

kevaundray · 2022-10-16T22:31:11Z

As of two minutes ago, I think we now have enough information to come to a conclusion on how to proceed with this

specs/eip4844/polynomial-commitments.md

…pt hash to second Fiat-Shamir stage in eval protocol

specs/eip4844/polynomial-commitments.md

kevaundray

Except description of BLS Modulus. Looks good to me.

Have left some optional house-keeping comments and notes.

specs/eip4844/polynomial-commitments.md

The barycentric comment error was found here: protolambda/go-kzg#25 (comment)

hwwhww

Great work on extracting the crypto lib helpers!

Some minor suggestions. Signing my approval stamp so that @asn-d6 can merge it.

specs/eip4844/polynomial-commitments.md

hwwhww · 2022-11-02T04:20:51Z

specs/eip4844/polynomial-commitments.md

+    # Generate random linear combination challenges
+    r = hash_to_bls_field(blobs, kzg_commitments)
+    r_powers = compute_powers(r, len(kzg_commitments))
+    r2 = int(r_powers[-1]) * r % BLS_MODULUS


Is there any better name than r2? 😅 You also call the return value r2 in the caller function.

It seems a bit confusing that we use r for the result (blob_to_polynomial) and the random number (r2) in this file.

Did an attempt at this in cb46b11

specs/eip4844/polynomial-commitments.md

hwwhww · 2022-11-02T04:36:04Z

setup.py

 def retrieve_blobs_sidecar(slot: Slot, beacon_block_root: Root) -> BlobsSidecar:
-    pass'''
+    return "TEST"'''


return type is wrong. how about:

def retrieve_blobs_sidecar(slot: Slot, beacon_block_root: Root) -> Optional[BlobsSidecar]: return None # Testing'''

Fixed in af48987

Hmm, slightly annoying because None could also mean that the sidecar is not available (I know in python we would do this by raising an exception but not all languages would have that available).

specs/eip4844/polynomial-commitments.md

hwwhww · 2022-11-02T04:53:43Z

specs/eip4844/polynomial-commitments.md

+
+| Name | Value |
+| - | - |
+| `BYTES_PER_FIELD_ELEMENT` | `uint64(32)` |


Does bytes_to_bls_field function make BYTES_PER_FIELD_ELEMENT must be 32?

Hmm, not sure exactly what you mean here. BYTES_PER_FIELD_ELEMENT being 32 is essentially a parameter of the BLS field we are using.

my 2c.
I think the point is that if you specify BYTES_PER_FIELD_ELEMENT as a preset, you should expect different values to be potentially configured. But thebytes_to_bls_field only works if BYTES_PER_FIELD_ELEMENT is strictly 32.
So I see two options:

BYTES_PER_FIELD_ELEMENT becomes a constant

generalize bytes_to_bls_field

my 2c. I think the point is that if you specify BYTES_PER_FIELD_ELEMENT as a preset, you should expect different values to be potentially configured. But thebytes_to_bls_field only works if BYTES_PER_FIELD_ELEMENT is strictly 32. So I see two options:

1. `BYTES_PER_FIELD_ELEMENT` becomes a constant 2. generalize `bytes_to_bls_field`

Ah, that makes sense! Thanks!

Seems like BYTES_PER_FIELD_ELEMENT was added as a preset in cbc170b, but I agree that it's not really a configurable value. Maybe it was done as a quick way to fix the tests if I read the commit correctly.

I mean like in theory we could encode field elements in 64 bytes or whatever, but I don't see why we would ever do that.

I think it's a good idea to leave it as a constant (which it already seems to be in polynomial-commitments.md).

hwwhww · 2022-11-02T04:57:45Z

specs/eip4844/polynomial-commitments.md

+#### `evaluate_polynomial_in_evaluation_form`
+
+```python
+def evaluate_polynomial_in_evaluation_form(polynomial: Polynomial,


[not-strong suggestion] Since Polynomial is defined as "a polynomial in evaluation form", I think you can make this function name shorter, e.g., evaluate_polynomial or evaluate_polynomial_at_point.

No strong opinion either. Inertia won and I left it as is.

kevaundray · 2022-11-02T14:43:34Z

Where is ENDIANNESS defined?

asn-d6 · 2022-11-02T17:38:45Z

Where is ENDIANNESS defined?

https://github.com/ethereum/annotated-spec/blob/master/phase0/beacon-chain.md#constants

asn-d6 · 2022-11-03T15:01:44Z

Merged! Thanks everyone!

asn-d6 and others added 4 commits October 14, 2022 16:09

4844: Start moving cryptography functions to polynomial-commitments

429e597

Co-authored-by: Kevaundray Wedderburn <kevtheappdev@gmail.com>

4844: hash_to_bls_field() doesn't use SSZ and does domain separation

22a4dcd

Co-authored-by: Kevaundray Wedderburn <kevtheappdev@gmail.com>

4844: Refactor aggregation and move to polynomial-commitments

b5959a1

Co-authored-by: Kevaundray Wedderburn <kevtheappdev@gmail.com>

4844: Fix ToC

642f138

Co-authored-by: Kevaundray Wedderburn <kevtheappdev@gmail.com>

asn-d6 force-pushed the 3030_rebased branch from 07bc7f7 to 642f138 Compare October 14, 2022 22:31

asn-d6 changed the title ~~PR 3030 rebased and improved~~ EIP4844: Update cryptography API and Fiat-Shamir logic Oct 14, 2022

asn-d6 commented Oct 14, 2022

View reviewed changes

specs/eip4844/polynomial-commitments.md Outdated Show resolved Hide resolved

asn-d6 commented Oct 14, 2022

View reviewed changes

specs/eip4844/polynomial-commitments.md Outdated Show resolved Hide resolved

asn-d6 commented Oct 14, 2022

View reviewed changes

specs/eip4844/polynomial-commitments.md Show resolved Hide resolved

kevaundray mentioned this pull request Oct 14, 2022

Removes SSZ and updates the crypto API #3030

Closed

hwwhww added 2 commits October 15, 2022 08:08

Merge branch 'dev' into pr3038

ff528a2

Fix linter error

91476fe

djrtwo reviewed Oct 16, 2022

View reviewed changes

ethereum deleted a comment from Where-2 Oct 16, 2022

ethereum deleted a comment from Where-2 Oct 17, 2022

dgcoffman mentioned this pull request Oct 19, 2022

EIP-4844 dgcoffman/lodestar#1

Draft

25 tasks

xrchz reviewed Oct 19, 2022

View reviewed changes

specs/eip4844/polynomial-commitments.md Show resolved Hide resolved

dankrad added 4 commits October 20, 2022 23:04

Blobs as flat ByteVector

090dc7e

Move is_data_available from validator to beacon_chain

30d19a3

Set domain separators to empty string

0eb82cf

Add placeholder domain separators for two protocols; add old transcri…

7631c18

…pt hash to second Fiat-Shamir stage in eval protocol

kevaundray reviewed Oct 22, 2022

View reviewed changes

specs/eip4844/polynomial-commitments.md Outdated Show resolved Hide resolved

dankrad added 2 commits October 22, 2022 15:12

Remove duplicate hash in

fe7af4b

Fix typo

46b6b24

kevaundray reviewed Oct 30, 2022

View reviewed changes

specs/eip4844/polynomial-commitments.md Outdated Show resolved Hide resolved

kevaundray approved these changes Oct 30, 2022

View reviewed changes

jessepollak mentioned this pull request Oct 31, 2022

EIP-4844 Implementers' Call #2 ethereum/pm#650

Closed

asn-d6 mentioned this pull request Nov 1, 2022

Rework the testing KZG trusted setups presets #3016

Merged

1 task

asn-d6 commented Nov 1, 2022

View reviewed changes

specs/eip4844/polynomial-commitments.md Outdated Show resolved Hide resolved

asn-d6 added 3 commits November 1, 2022 15:55

Improve comments in polynomial-commitments.md

80d4d09

The barycentric comment error was found here: protolambda/go-kzg#25 (comment)

Fix description of BLS_MODULUS

c8b8b53

blob_to_field_elements() -> blob_to_polynomial()

5354a96

hwwhww mentioned this pull request Nov 1, 2022

EIP4844 spec tests #3074

Closed

5 tasks

timbeiko mentioned this pull request Nov 1, 2022

EIP-4844 Implementers' Call #3 ethereum/pm#655

Closed

asn-d6 added 2 commits November 1, 2022 19:04

Use modular multiplication in compute_aggregated_poly_and_commitment

0e2e477

Satisfy executable spec tests

db619e2

asn-d6 force-pushed the 3030_rebased branch from 2e5a784 to db619e2 Compare November 1, 2022 17:05

kevaundray approved these changes Nov 1, 2022

View reviewed changes

hwwhww approved these changes Nov 2, 2022

View reviewed changes

asn-d6 added 2 commits November 2, 2022 12:19

Improve code that deals with the evaluation challenge

cb46b11

Minor spec improvements (after review by hww)

af48987

dankrad and others added 3 commits November 3, 2022 13:40

Remove extra space

1c9a8db

Move BYTES_PER_FIELD_ELEMENT from presets to constants

186a2eb

Clarify BYTES_PER_FIELD_ELEMENT comment a bit more

c130995

asn-d6 merged commit 86e1576 into ethereum:dev Nov 3, 2022

This was referenced Nov 3, 2022

EIP4844: Suggestion to remove ssz from the Fiat-Shamir function #3026

Closed

EIP4844: Add check to ensure Blobs are canonical #3057

Closed

zilm13 mentioned this pull request Nov 10, 2022

Add\update containers Consensys/teku#6383

Closed

6 tasks

asn-d6 mentioned this pull request Nov 18, 2022

EIP4844: Update crypto API (simplify precompile / sync with consensus-specs) ethereum/EIPs#6002

Merged

protolambda mentioned this pull request Nov 22, 2022

Shanghai Planning ethereum/pm#450

Closed

hwwhww mentioned this pull request May 15, 2023

Update EIP-4844: Decouple blobs ethereum/EIPs#6610

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP4844: Update cryptography API and Fiat-Shamir logic #3038

EIP4844: Update cryptography API and Fiat-Shamir logic #3038

asn-d6 commented Oct 14, 2022 •

edited

Loading

kevaundray commented Oct 16, 2022

djrtwo left a comment

kevaundray commented Oct 16, 2022

kevaundray commented Oct 16, 2022

kevaundray left a comment •

edited

Loading

hwwhww left a comment

hwwhww Nov 2, 2022

asn-d6 Nov 2, 2022

hwwhww Nov 2, 2022

asn-d6 Nov 2, 2022

dankrad Nov 3, 2022

hwwhww Nov 2, 2022 •

edited

Loading

asn-d6 Nov 2, 2022

tbenr Nov 3, 2022

asn-d6 Nov 3, 2022 •

edited

Loading

hwwhww Nov 2, 2022

asn-d6 Nov 2, 2022

kevaundray commented Nov 2, 2022

asn-d6 commented Nov 2, 2022

asn-d6 commented Nov 3, 2022

EIP4844: Update cryptography API and Fiat-Shamir logic #3038

EIP4844: Update cryptography API and Fiat-Shamir logic #3038

Conversation

asn-d6 commented Oct 14, 2022 • edited Loading

kevaundray commented Oct 16, 2022

djrtwo left a comment

Choose a reason for hiding this comment

kevaundray commented Oct 16, 2022

kevaundray commented Oct 16, 2022

kevaundray left a comment • edited Loading

Choose a reason for hiding this comment

hwwhww left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asn-d6 Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevaundray commented Nov 2, 2022

asn-d6 commented Nov 2, 2022

asn-d6 commented Nov 3, 2022

asn-d6 commented Oct 14, 2022 •

edited

Loading

kevaundray left a comment •

edited

Loading

hwwhww Nov 2, 2022 •

edited

Loading

asn-d6 Nov 3, 2022 •

edited

Loading